Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: specify an ovn underlay network through MicroOVN #361

Merged
merged 10 commits into from
Sep 6, 2024

Conversation

gabrielmougard
Copy link
Contributor

closes #239

@gabrielmougard gabrielmougard force-pushed the feat/ovn-east-west-integration branch 9 times, most recently from 863f7e7 to 27c196b Compare August 2, 2024 14:43
@gabrielmougard gabrielmougard marked this pull request as ready for review August 2, 2024 15:01
Copy link
Contributor

@masnax masnax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, hope the refactor wasn't too annoying. I Just have a few nits.

cmd/microcloud/ask.go Outdated Show resolved Hide resolved
service/lxd.go Show resolved Hide resolved
@gabrielmougard gabrielmougard force-pushed the feat/ovn-east-west-integration branch 3 times, most recently from a75a99d to aa21322 Compare August 5, 2024 07:55
doc/explanation/microcloud.md Show resolved Hide resolved
doc/explanation/microcloud.md Show resolved Hide resolved
doc/how-to/initialise.md Show resolved Hide resolved
doc/how-to/ovn_underlay.md Show resolved Hide resolved
doc/how-to/ovn_underlay.md Show resolved Hide resolved
doc/how-to/ovn_underlay.md Show resolved Hide resolved
doc/how-to/initialise.md Show resolved Hide resolved
doc/how-to/ovn_underlay.md Show resolved Hide resolved
doc/how-to/ovn_underlay.md Show resolved Hide resolved
doc/how-to/ovn_underlay.md Show resolved Hide resolved
@gabrielmougard gabrielmougard force-pushed the feat/ovn-east-west-integration branch 4 times, most recently from 27ff7f0 to fbf7758 Compare August 20, 2024 13:19
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
@gabrielmougard gabrielmougard force-pushed the feat/ovn-east-west-integration branch 2 times, most recently from ecba06c to 48d9bf0 Compare August 20, 2024 16:44
cmd/microcloud/main_init.go Outdated Show resolved Hide resolved
@gabrielmougard gabrielmougard force-pushed the feat/ovn-east-west-integration branch 3 times, most recently from 1a9cf4a to 867c0e3 Compare August 22, 2024 06:19
@gabrielmougard
Copy link
Contributor Author

@masnax it seems to solve the issue. But now the interactive combination tests seem to fail at some point.

@masnax
Copy link
Contributor

masnax commented Aug 22, 2024

@gabrielmougard Needs a rebase.

@gabrielmougard gabrielmougard force-pushed the feat/ovn-east-west-integration branch 2 times, most recently from e6edc44 to 2d783ee Compare August 23, 2024 12:20
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
test/suites/basic.sh Fixed Show fixed Hide fixed
@gabrielmougard gabrielmougard force-pushed the feat/ovn-east-west-integration branch 10 times, most recently from 25ec1bd to 1521d31 Compare September 5, 2024 09:50
Signed-off-by: Gabriel Mougard <[email protected]>
This will be needed to check the API extensions of different services from MicroCloud

Signed-off-by: Gabriel Mougard <[email protected]>
@gabrielmougard gabrielmougard force-pushed the feat/ovn-east-west-integration branch 4 times, most recently from 0402aef to dbba372 Compare September 5, 2024 11:10
Signed-off-by: Gabriel Mougard <[email protected]>
@gabrielmougard
Copy link
Contributor Author

@gabrielmougard it looks like the basic tests aren't passing because the test_add_services set of tests don't have an expected answer for the ovn_underlay_network when calling service add.

With the changed behaviour, microcloud init should ask about the underlay network, but running microcloud service add should skip it, so in each test case you need to set and unset the environment variables accordingly.

It would be helpful to also add a test to cover the case where you run microcloud add in the following situation:

  • microcloud init was run to set up 2 nodes with MicroOVN uninstalled (so setup for it was skipped)
  • MicroOVN was then installed on both systems (but not initialized)
  • microovn cluster bootstrap was run on a "new" 3rd node
  • microcloud add was called to add this new 3rd node.

In this case, we should verify that MicroCloud offers micro01 and micro02 to add their underlay configuration, but skips micro03 since it was already set up, even though technically micro03 is the one joining the overall MicroCloud cluster.

I added the test case @masnax Tell me what you think about it ;)

Copy link
Contributor

@masnax masnax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The preseed logic seems to be incorrect, but it's a small fix and since you're going away for a couple weeks, I'll merge this and make a follow-up PR. I'll tag you so you can see how the preseed IPs should be determined.

We also need to update the preseed tests for the new keys, and add the new preseed key to the docs as well.

Comment on lines +570 to +576
for peer, system := range c.state {
if len(system.AvailableOVNInterfaces) == 0 {
fmt.Printf("Not enough interfaces available on %s to create an underlay network, skipping\n", peer)
canOVNUnderlay = false
break
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c.state is not populated for preseed since the recipe is precise, we don't want to change any existing system configurations using a preseed file.

Comment on lines +608 to +611
ip, _, err := net.ParseCIDR(underlays[peer])
if err != nil {
return nil, fmt.Errorf("Failed to parse underlay IP: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input is not a cidr, so this will fail.

@masnax masnax merged commit a26a916 into canonical:main Sep 6, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add option for specifying the network to use for OVN east-west (underlay) traffic
4 participants